Add IBMCloud manual mode #356
Conversation
Support the IBMCloud platform in manual mode. Update ccoctl to handle IBMCloud credentialsrequests by essentially emulating Passthrough mode. An API key environment variable is used to populate a specific field in the secret.
|
/retest |
dgoodwin
left a comment
dgoodwin
left a comment
There was a problem hiding this comment.
Looks pretty good to me for a first cut until we can get defined permissions in there. Just a couple minor requests.
Co-authored-by: Devan Goodwin <[email protected]>
|
@dgoodwin thanks for the review. I pushed up a couple of commits to fix things up. |
|
/lgtm |
openshift-ci
bot
added
lgtm
| credreqv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1" | ||
| "github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning" |
There was a problem hiding this comment.
To keep consistency, these imports should come in a separate block at the end
There was a problem hiding this comment.
Done. Hopefully I understood you correctly.
| // NewCreateCmd implements the "create" command for the credentials provisioning | ||
| func NewCreateCmd() *cobra.Command { | ||
| createCmd := &cobra.Command{ | ||
| Use: "create", |
There was a problem hiding this comment.
by command conventions that we follow, this should be create-secrets I feel. @dgoodwin WDYT?
There was a problem hiding this comment.
That's no problem. Changed to create-secrets
| { | ||
| name: "Generate Secret for one CredentialsRequest", | ||
| setup: func(t *testing.T) string { | ||
| tempDirName, err := ioutil.TempDir(os.TempDir(), testDirPrefix) | ||
| require.NoError(t, err, "Failed to create temp directory") | ||
|
|
||
| err = testCredentialsRequest(t, "firstcredreq", "namespace1", "secretName1", tempDirName) | ||
| require.NoError(t, err, "Errored while setting up test CredReq files") | ||
|
|
||
| return tempDirName | ||
| }, | ||
| verify: func(t *testing.T, targetDir string) { | ||
| files, err := ioutil.ReadDir(targetDir) | ||
| require.NoError(t, err, "Unexpected error listing files in targetDir") | ||
|
|
||
| assert.Equal(t, 1, len(files), "Should be exactly 1 Secret generated for 1 CredentialsRequest") | ||
| }, | ||
| cleanup: func(t *testing.T) { | ||
| return | ||
| }, | ||
| expectError: false, |
There was a problem hiding this comment.
Can we have a test that does setting IC_API_KEY in setup and verify if generated secret actually has that key? Also, one negative test where env var is not set and error is thrown.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akhil-rane, BobbyRadford, dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Ran into this on a very recent Installer PR, but seems like AWS e2e tests are failing due to some throttling issues. We ended up getting GCP tests to run instead. Is that something we can do here to progress this PR? @akhil-rane @dgoodwin openshift/installer#4923 (comment) |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
| // NewCreateSecretsCmd implements the "create-secrets" command for the credentials provisioning | ||
| func NewCreateSecretsCmd() *cobra.Command { | ||
| createSecretsCmd := &cobra.Command{ | ||
| Use: "create-secrets", |
There was a problem hiding this comment.
@BobbyRadford I was discussing this with the team and I was wondering if a name like create-shared-secrets would be better.
I'm comparing this to the existing aws support and there is no equivalent create-secrets subcommand (today) for AWS (which isn't surprising b/c of lack of support for passthrough-equivalent mode). So this subcommand feels specific to implementing Passthrough mode and the name of the command should reflect that.
Could we rename this to something like create-shared-secrets and have the long description make it clear that this subcommand just takes the API key from an env var and uses it to satisfy all the CredentialsRequests?
There was a problem hiding this comment.
Hey @joelddiaz no problem. Will make that happen.
Add IBMCloud manual mode
6 participants
Support the IBMCloud platform in Manual mode for IPI/UPI.
Related to #344 and openshift/enhancements#773